MrBean: Fix detection of inherited default method in Java 8+ interface #102

robbytx · 2020-09-03T02:15:46Z

Problem

When the type to materialize extends/implements some interface that has a default method implementation (with a non-bean method name), MrBean will treat the method as if it lacks an implementation.

Solution

Use Class.getMethod to locate the exact method that the implemented type will utilize, which properly handles this case.

Verification

I've added several unit tests to cover the fixed behavior, and to verify existing, related behavior:

TestSimpleMaterializedInterfaces.testInheritedDefaultMethodInInterface is the test case that exercise the current issue, which is now resolved.
TestSimpleMaterializedInterfaces.testDefaultMethodInInterface is a simpler test case that is already passing with the existing implementation.
TestAbstractClasses and TestAbstractClassesWithOverrides have been extended to exercise some cases that justify the original code in hasConcreteOverride. These test cases were already passing with the existing implementation.

…efault methods in Java 8+ interfaces

robbytx · 2020-09-03T02:54:37Z

mrbean/src/main/java/com/fasterxml/jackson/module/mrbean/BeanBuilder.java

        for (JavaType curr = implementedType; (curr != null) && !curr.isJavaLangObject();
                curr = curr.getSuperClass()) {
            // 29-Nov-2015, tatu: Avoiding exceptions would be good, so would linear scan
            //    be better here?
            try {
                Method effectiveMethod = curr.getRawClass().getDeclaredMethod(name, argTypes);
-                if (effectiveMethod != null && BeanUtil.isConcrete(effectiveMethod)) {


getDeclaredMethod (like getMethod) never returns null

cowtowncoder · 2020-09-03T20:41:38Z

mrbean/src/test/java/com/fasterxml/jackson/module/mrbean/TestAbstractClasses.java

        public abstract String getX();

        public String getFoo() { return "Foo!"; }
        public void setY(int value) { y = value; }
+
+        // also verify non-public methods
+        protected abstract String getZ();


Not sure if we are expected to implement non-public getters... although probably makes sense (setters def. need to be implemented).

I wasn't sure whether non-public methods were supported until I wrote the test, which I decided to do after exploring the differences between getMethod and getDeclaredMethod. Once I realized that non-public methods were working the same as public ones, I figured it was worthwhile to create a test to verify that behavior wasn't regressed, per Hyrum's law.

cowtowncoder · 2020-09-03T20:48:11Z

mrbean/src/main/java/com/fasterxml/jackson/module/mrbean/BeanBuilder.java

@@ -176,16 +176,28 @@ protected boolean hasConcreteOverride(Method m0, JavaType implementedType)
    {
        final String name = m0.getName();
        final Class<?>[] argTypes = m0.getParameterTypes();
+        try {


I guess I am specifically trying to understand this part here; why is the extra check needed?
Is that because existing code seems to only look for supertypes (and not type itself) or something else?

It's a bit convoluted (and took me some time to spot the issue), so let me illustrate with the BeanWithInheritedDefault example from the unit test:

In BeanBuilder.implement,ArrayList<JavaType> implTypes contains [BeanWithInheritedDefault, BeanWithDefaultForOtherInterface, Bean, OtherInterface], and these reduce two relevant executions of the loop with Method m:

BeanWithDefaultForOtherInterface.anyValuePresent, which is concrete itself, and is properly skipped at line 108

OtherInterface.anyValuePresent, which is not, and therefore which calls the hasConcreteOverride method I've modified.

In the original implementation of hasConcreteOverride, it checks for a concrete declared method on the implementedType (in this case BeanWithInheritedDefault, which doesn't have it), and then it checks on superclasses of which there is none. Critically, this code fails to locate the concrete implementation of this method in BeanWithDefaultForOtherInterface, and therefore it incorrectly return false from the method.

I started to explore solutions that would either (a) traverse the interface hierarchy, or (b) optimize the loop in implement to track known-concrete methods, but I ultimately realized this simple addition to hasConcreteOverride solves the search problem for all public methods, which coincidentally applies to all Java interfaces and their default methods. I think it also makes the hasConcreteOverride more efficient, since it is only necessary to search the superclass hierarchy when the method is in question is non-public.

Ah ok. So the logic was ok pre-Java8, but the introduction of interface default implementations threw the monkey wrench... makes sense.

cowtowncoder · 2020-09-03T20:53:56Z

First of all, thank you very much for contributing this patch!
I'll need to understand this a bit better, but I assume it makes sense and solves an existing problem.

Couple of practical things:

If I havent' asked for and received CLA (see https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf), we would need that. The simplest way is usually to print, fill & sign, scan/take photo, email to info at fasterxml dot com
Would it be possible to separate unit tests and fix -- this because I will need to figure out a way to merge this properly wrt 2.12 and master

The challenge with (2) would be Java 8; I want the fix in 2.12 but that can only use Java 8.
Actually... since build already requires Java 8, it might be ok to have that as test-only dependency (that is, tests can use Java 8 default implementation; just not non-test code). I'll have to verify this works wrt maven source/target build levels.

If you can create separate PR(s), making those against 2.12 would be great (at least for fixes part; tests I can probably merge manually if need be).

robbytx · 2020-09-04T20:39:07Z

I'll send the CLA over the next few days. I'm checking if I need approval from someone in my company.
I will go ahead and create these, so you can merge once you receive my CLA.

robbytx · 2020-09-04T21:13:03Z

Ok... I've created separate PRs for (103) the fix, (104) passing tests in 2.12, (105) both of those plus the test for the fix, all against 2.12.

I'll hopefully deliver my CLA next week, or let you know if it is delayed for some reason.

robbytx · 2020-09-11T19:34:47Z

CLA is still pending... it took until today to get it in front of the right people for review. I'll keep you posted next week.

robbytx · 2020-09-18T16:43:04Z

@cowtowncoder I've sent our Corporate CLA to the address you specified. I've also created a fork from our organization, and pushed my branch there.

Are you able to change this PR to merge from https://github.com/bazaarvoice/jackson-modules-base/tree/test-default-in-interfaces instead? I don't seem to have that permission. If necessary, I can create a new PR, but I was hoping to make the change here to retain the existing context.

cowtowncoder · 2020-09-20T03:00:44Z

@robbytx Ok I am finally back looking at this: apologies for slow follow-up. CCLA is in place and all.

It looks like this PR and 103, 104 and 105 are overlapping, plus you mentioned that it'd make sense to rather merge from BV repo.
So, I created #108 -- is that what you meant? It's against master but I should be able to cherry-pick it to go in 2.12(.0) as well -- hoping to finalize 2.12.0-pr1 within next week or so.

robbytx · 2020-09-20T17:43:49Z

@cowtowncoder No problem at all on follow-up time -- you've been waiting much longer for me for CLA.

Also, I didn't intend for you to have to create a new PR from the BV repo -- I was hoping that you were able to simply edit this PR to switch the source to that repo. I know this is possible to do within a single organization, but GitHub is not allowing me to update my PRs in this repo, and I thought you might be able to do that.

Finally, I'm sorry for my confusion on which branch(es) to target... I should have read CONTRIBUTING earlier.

If possible, I would love for my patches (which I believe are purely bug fixes) to go into a new patch version on 2.11. If that's not appropriate, I definitely want them to go into 2.12, and of course ultimately master.

To recap, here's a quick summary of the current commits/branches for this patch:

73b8f0c (fix + tests) targets master, can merge from bazaarvoice:test-default-in-interfaces - this PR
ce78572 (fix only) targets 2.12, can merge from bazaarvoice:backport-fix - matches MrBean: Fix detection of inherited default method in Java 8+ interface #103
3e15ced (tests for existing functions only) targets 2.12, can merge from bazaarvoice:backport-passing-tests - matches MrBean (tests): Verify behavior of default methods in interfaces, non-public methods in classes #104
b081a18 (fix + all tests, includes 2, 3 and test for 2) targets 2.12, can merge from bazaarvoice:backport-test-for-fix - matches MrBean: Fix detection of inherited default method in Java 8+ interface #105

It seems like (4) is what you should ultimately merge into 2.12, although I can recreate that PR to originate from the BV repo. The only reason I created (2) and (3) is that you had previously asked to separate the patch into one for the fix and one for the tests.

At this point:

I would like your input on whether this patch is a candidate for the 2.11 branch. If so, I can create an equivalent PR, or you can simply cherry-pick my 2.12 changes if that would work.
I will recreate my PRs against 2.12, but I would like your input whether separate PRs (2, 3, and 4) to 2.12 are still desirable, or if you will just merge (4).
I think this PR can closed as long as my patch is going into 2.12, which I presume will ultimately get merged back into master along with other 2.12 changes.

cowtowncoder · 2020-09-21T22:32:17Z

@robbytx Ok, that makes sense. So, on 2.11 vs 2.12... it's a judgment call: since there is no API change, this could go in 2.11 (or even in 2.10 if there was specific need). My only concern is wrt risk, how likely it is that something else might break.
I don't see anything super risky per se but I also know that mr bean test coverage is limited to catching problems seen before, for a small number of use cases.

But I think that with all of that, 2.11 would be fine.

robbytx · 2020-09-22T21:21:45Z

Closing this in favor of #109, which targets 2.11.

robbytx added 5 commits September 2, 2020 20:21

Verify default methods in interface are supported

498cac3

Add failing test case

22b8550

Verify non-public class method behavior

66df757

Verify current behavior of non-public methods

f8a9a0d

Use Class.getMethod to locate the precise implementation, including d…

73b8f0c

…efault methods in Java 8+ interfaces

robbytx commented Sep 3, 2020

View reviewed changes

cowtowncoder reviewed Sep 3, 2020

View reviewed changes

robbytx mentioned this pull request Sep 20, 2020

Avoid generating properties based on synthetic bridge methods #106

Closed

robbytx mentioned this pull request Sep 22, 2020

MrBean: Fix detection of inherited default method in Java 8+ interface #109

Merged

robbytx closed this Sep 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

MrBean: Fix detection of inherited default method in Java 8+ interface #102

MrBean: Fix detection of inherited default method in Java 8+ interface #102

Uh oh!

robbytx commented Sep 3, 2020

Uh oh!

robbytx Sep 3, 2020

Uh oh!

cowtowncoder Sep 3, 2020

Uh oh!

robbytx Sep 4, 2020

Uh oh!

cowtowncoder Sep 3, 2020

Uh oh!

robbytx Sep 4, 2020

Uh oh!

cowtowncoder Sep 20, 2020

Uh oh!

cowtowncoder commented Sep 3, 2020

Uh oh!

robbytx commented Sep 4, 2020

Uh oh!

robbytx commented Sep 4, 2020

Uh oh!

robbytx commented Sep 11, 2020

Uh oh!

robbytx commented Sep 18, 2020

Uh oh!

cowtowncoder commented Sep 20, 2020

Uh oh!

robbytx commented Sep 20, 2020

Uh oh!

cowtowncoder commented Sep 21, 2020

Uh oh!

robbytx commented Sep 22, 2020

Uh oh!

Uh oh!

MrBean: Fix detection of inherited default method in Java 8+ interface #102

MrBean: Fix detection of inherited default method in Java 8+ interface #102

Uh oh!

Conversation

robbytx commented Sep 3, 2020

Problem

Solution

Verification

Uh oh!

robbytx Sep 3, 2020

Choose a reason for hiding this comment

Uh oh!

cowtowncoder Sep 3, 2020

Choose a reason for hiding this comment

Uh oh!

robbytx Sep 4, 2020

Choose a reason for hiding this comment

Uh oh!

cowtowncoder Sep 3, 2020

Choose a reason for hiding this comment

Uh oh!

robbytx Sep 4, 2020

Choose a reason for hiding this comment

Uh oh!

cowtowncoder Sep 20, 2020

Choose a reason for hiding this comment

Uh oh!

cowtowncoder commented Sep 3, 2020

Uh oh!

robbytx commented Sep 4, 2020

Uh oh!

robbytx commented Sep 4, 2020

Uh oh!

robbytx commented Sep 11, 2020

Uh oh!

robbytx commented Sep 18, 2020

Uh oh!

cowtowncoder commented Sep 20, 2020

Uh oh!

robbytx commented Sep 20, 2020

Uh oh!

cowtowncoder commented Sep 21, 2020

Uh oh!

robbytx commented Sep 22, 2020

Uh oh!

Uh oh!